Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added ability to init SearchQuery props on construction #546

Closed
wants to merge 1 commit into from
Closed

Added ability to init SearchQuery props on construction #546

wants to merge 1 commit into from

Conversation

Enchiridion
Copy link

Pull Request

What does this PR do?

This lets you init the SearchQuery props when the object is constructed. I think the PHP library is the only one that doesn't yet let you do this or use a simple array instead. I created this because I was using the search() method but needed to switch to multiSearch(), but found it works completely differently, this speeds up using it.

Previous way:

$data = (new SearchQuery())
   ->setIndexUid('books')
   ->setQuery('butler')
   ->setSort(['author:desc']);

Additional way:

$data = new SearchQuery([
    'indexUid' => 'books',
    'q'        => 'butler',
    'sort'     => ['author:desc'],
]);

Note 1: My change currently does not pass the test due to the use of variable property access. However I think it's safe because all the props are simple and typed. If you think that's ok, I can have phpstan ignore that line.

 ------ -----------------------------------------------------------------------
  Line   src/Contracts/SearchQuery.php
 ------ -----------------------------------------------------------------------
  33     Variable property access on $this(Meilisearch\Contracts\SearchQuery).
 ------ -----------------------------------------------------------------------

Note 2: The initial code I based this on from main currently does not pass all the phpstan tests:

 ------ ------------------------------------------------------------------------------
  Line   tests/Endpoints/DocumentsTest.php
 ------ ------------------------------------------------------------------------------
  324    Call to an undefined method
         PHPUnit\Framework\MockObject\Rule\InvokedCount::numberOfInvocations().
  805    Call to an undefined method
         PHPUnit\Framework\MockObject\Rule\InvokedAtLeastOnce::numberOfInvocations().
 ------ ------------------------------------------------------------------------------

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

@Enchiridion
Copy link
Author

I added in the linter ignore for the Variable property access line.

@Enchiridion Enchiridion marked this pull request as ready for review July 11, 2023 02:32
@norkunas
Copy link
Collaborator

Is it really worth? With array you don't have autocompletion, because the structure is undefined

@norkunas
Copy link
Collaborator

I added in the linter ignore for the Variable property access line.

I'd document the structure instead of ignoring

@Enchiridion
Copy link
Author

I've updated the PR so all the properties are in the constructor. With PHP8+ you can specify the args using named arguments along with array spreading to get the same effect of setting all those properties quickly and easily using an array.

norkunas
norkunas requested changes Aug 15, 2023
View reviewed changes
src/Contracts/SearchQuery.php
int $hitsPerPage = null,
int $page = null,
) {
foreach (get_defined_vars() as $name => $value) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set the vars manually then the phpstan won't complain and in future it could be refactored to CPP

@curquiza
Copy link
Member

@Enchiridion do you still plan to work on this PR? 😊

@curquiza
Copy link
Member

curquiza commented Oct 3, 2024

Closing this for lack of activity. Feel free to re-open of course

@curquiza curquiza closed this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@norkunas norkunas norkunas requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Enchiridion @norkunas @curquiza